-
-
Notifications
You must be signed in to change notification settings - Fork 4
Show relevant context when recording audio #1967
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…prop on primitive editors.
…ense` or `Example` and choses the related editor
…itors can get the current object being edited
…to be used inline and not via dialogs-service.ts
…n the audio dialog
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/viewer/src/lib/components/audio/AudioDialog.svelte (1)
55-66
: Surface upload errors to users and prevent double-submitCurrently thrown errors bubble with no user feedback. Catch and toast; also disable while submitting.
async function submitAudio() { - if (!selectedFile) throw new Error('No audio to upload'); + if (!selectedFile) { + AppNotification.display($t`No audio to upload`, { type: 'error' }); + return; + } submitting = true; - try { + try { const audioId = await uploadAudio(); - onSubmit(audioId); + await onSubmit(audioId); close(); + } catch (err) { + const message = err instanceof Error ? err.message : $t`Unknown error`; + AppNotification.display($t`Failed to save audio: ${message}`, { type: 'error' }); } finally { submitting = false; } }Additionally, update the Save button (outside this hunk) to also disable during submit:
<Button onclick={() => submitAudio()} disabled={submitting || tooBig || !finalAudio} loading={submitting}> {$t`Save audio`} </Button>
🧹 Nitpick comments (22)
frontend/viewer/src/lib/layout/DevToolsDialog.svelte (1)
9-12
: Guard API usage and optionally batch-create in parallel
- Add a quick guard for missing
api
to avoid a runtime if Dev Tools is opened without a project.- Minor: for dev productivity, consider parallelizing creation.
export async function generateEntries(n: number) { - for (let i = 0; i < n; i++) { - const entry = defaultEntry(); - const vWsId = writingSystems.defaultVernacular?.wsId; - if (vWsId) entry.citationForm[vWsId] = `*Test ${Math.random().toString(36).substring(2, 7)}`; - await projectContext.api.createEntry(entry); - } + if (!projectContext.maybeApi) return; // or show a toast + const tasks: Promise<unknown>[] = []; + for (let i = 0; i < n; i++) { + const entry = defaultEntry(); + const vWsId = writingSystems.defaultVernacular?.wsId; + if (vWsId) entry.citationForm[vWsId] = `*Test ${Math.random().toString(36).substring(2, 7)}`; + tasks.push(projectContext.api.createEntry(entry)); + } + await Promise.all(tasks); }Also applies to: 20-27
frontend/viewer/src/project/browse/BrowseView.svelte (1)
21-24
: Right pane renders empty when API is unavailable—show a fallbackIf
projectContext.maybeApi
is falsy and an entry is selected, the pane is empty. Consider a lightweight message.- {:else if projectContext.maybeApi} + {:else if projectContext.maybeApi} <div class="md:p-4 md:pl-4 h-full"> <EntryView entryId={selectedEntryId.current} onClose={() => (selectedEntryId.current = '')} showClose={IsMobile.value} /> </div> + {:else} + <div class="flex items-center justify-center h-full text-muted-foreground text-center m-2"> + <p>{$t`Project API unavailable. Please try again or reload.`}</p> + </div> {/if}Also applies to: 106-114
frontend/viewer/src/lib/components/audio/AudioDialog.svelte (1)
14-24
: Allow async onSubmit and await it before closingLet
onSubmit
be async-capable so parents can persist updates before the dialog closes.- let { + let { open = $bindable(false), title = undefined, - onSubmit = () => {}, + onSubmit = () => {}, children = undefined } : { open: boolean, title?: string, - onSubmit?: (audioId: string) => void, + onSubmit?: (audioId: string) => void | Promise<void>, children?: Snippet } = $props();try { const audioId = await uploadAudio(); - onSubmit(audioId); + await onSubmit(audioId); close(); } finally { submitting = false; }Also applies to: 61-62
frontend/viewer/src/lib/components/field-editors/audio-input.svelte (1)
226-230
: Explicitly close the dialog on submit (defensive).If AudioDialog doesn’t auto-close after onSubmit in all paths, the open state could remain true.
function onAudioDialogSubmit(newAudioId: string) { audioId = newAudioId; onchange(newAudioId); + audioDialogOpen = false; }
frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte (3)
38-48
: Avoid accidentally rendering the class "true".If $currentView.fields.sentence.show can be boolean, class={cn(show || 'hidden')} may yield class="true". Prefer an explicit ternary.
Apply:
- <Editor.Field.Root fieldId="sentence" class={cn($currentView.fields.sentence.show || 'hidden')}> + <Editor.Field.Root fieldId="sentence" class={cn($currentView.fields.sentence.show ? undefined : 'hidden')}>
49-58
: Same class boolean-guard nit as above.- <Editor.Field.Root fieldId="translation" class={cn($currentView.fields.translation.show || 'hidden')}> + <Editor.Field.Root fieldId="translation" class={cn($currentView.fields.translation.show ? undefined : 'hidden')}>
61-70
: Use Svelte’s class directive for boolean-guard
Replaceclass={cn($currentView.fields.reference.show || 'hidden')}with
class:hidden={!$currentView.fields.reference.show}frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte (7)
44-54
: Field wiring LGTM; apply the class boolean-guard nit.- <Editor.Field.Root fieldId="lexemeForm" class={cn($currentView.fields.lexemeForm.show || 'hidden')}> + <Editor.Field.Root fieldId="lexemeForm" class={cn($currentView.fields.lexemeForm.show ? undefined : 'hidden')}>
56-65
: Citation form block looks good; same class nit.- <Editor.Field.Root fieldId="citationForm" class={cn($currentView.fields.citationForm.show || 'hidden')}> + <Editor.Field.Root fieldId="citationForm" class={cn($currentView.fields.citationForm.show ? undefined : 'hidden')}>
68-76
: Complex forms section LGTM; same class nit.- <Editor.Field.Root fieldId="complexForms" class={cn($currentView.fields.complexForms.show || 'hidden')}> + <Editor.Field.Root fieldId="complexForms" class={cn($currentView.fields.complexForms.show ? undefined : 'hidden')}>
78-87
: Components section LGTM; same class nit.- <Editor.Field.Root fieldId="components" class={cn($currentView.fields.components.show || 'hidden')}> + <Editor.Field.Root fieldId="components" class={cn($currentView.fields.components.show ? undefined : 'hidden')}>
89-101
: Complex form types: consider minor perf hoists.The labelSelector calls pickBestAlternative per option each render. If options are stable, memoization/hoisting can cut work. Also apply the class nit.
- <Editor.Field.Root fieldId="complexFormTypes" class={cn($currentView.fields.complexFormTypes.show || 'hidden')}> + <Editor.Field.Root fieldId="complexFormTypes" class={cn($currentView.fields.complexFormTypes.show ? undefined : 'hidden')}>Outside this hunk, you can hoist:
const analysisWs = writingSystemService.viewAnalysis($currentView); const vernacularWs = writingSystemService.viewVernacular($currentView);…and reuse analysisWs/vernacularWs where referenced.
104-113
: Literal meaning block LGTM; same class nit.- <Editor.Field.Root fieldId="literalMeaning" class={cn($currentView.fields.literalMeaning.show || 'hidden')}> + <Editor.Field.Root fieldId="literalMeaning" class={cn($currentView.fields.literalMeaning.show ? undefined : 'hidden')}>
115-124
: Note block LGTM; same class nit.- <Editor.Field.Root fieldId="note" class={cn($currentView.fields.note.show || 'hidden')}> + <Editor.Field.Root fieldId="note" class={cn($currentView.fields.note.show ? undefined : 'hidden')}>frontend/viewer/src/lib/entry-editor/object-editors/LexiconEditorPrimitive.svelte (1)
14-20
: Render routing by type guards is clean.Optional: Provide a tiny fallback (e.g., empty state) for unexpected values to aid debugging in the audio dialog.
{:else if isExample(object)} <ExampleEditorPrimitive example={object} readonly /> {:else} -{/if} + <!-- no object / unknown type --> +{/if}frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte (3)
41-50
: Gloss field wiring LGTM; apply the class boolean-guard nit.- <Editor.Field.Root fieldId="gloss" class={cn($currentView.fields.gloss.show || 'hidden')}> + <Editor.Field.Root fieldId="gloss" class={cn($currentView.fields.gloss.show ? undefined : 'hidden')}>
52-61
: Definition field LGTM; same class nit.- <Editor.Field.Root fieldId="definition" class={cn($currentView.fields.definition.show || 'hidden')}> + <Editor.Field.Root fieldId="definition" class={cn($currentView.fields.definition.show ? undefined : 'hidden')}>
79-91
: Semantic domains LGTM; same class nit.- <Editor.Field.Root fieldId="semanticDomains" class={cn($currentView.fields.semanticDomains.show || 'hidden')}> + <Editor.Field.Root fieldId="semanticDomains" class={cn($currentView.fields.semanticDomains.show ? undefined : 'hidden')}>frontend/viewer/src/lib/entry-editor/object-editors/subject-context.ts (2)
4-8
: Consider generic typing for stronger narrowing.Making EditorPrimitiveSubject generic preserves the concrete subject type for consumers.
-interface EditorPrimitiveSubject { - readonly current: IEntry | ISense | IExampleSentence | undefined -} -export const subjectContext = new Context<EditorPrimitiveSubject>('subject-context'); +type Subject = IEntry | ISense | IExampleSentence; +interface EditorPrimitiveSubject<T extends Subject = Subject> { + readonly current: T | undefined; +} +export const subjectContext = new Context<EditorPrimitiveSubject>('subject-context');
9-17
: Tighten/relax types and drop unnecessary optional call.subject is required here, so subject?.() is unnecessary. Optionally allow undefined subjects to make the API reusable in wrappers.
-export function initSubjectContext(subject: () => IEntry | ISense | IExampleSentence) { +export function initSubjectContext(subject: () => IEntry | ISense | IExampleSentence | undefined) { const editorPrimitiveSubject: EditorPrimitiveSubject = { get current() { - return subject?.(); + return subject(); } }; subjectContext.set(editorPrimitiveSubject); return editorPrimitiveSubject; }frontend/viewer/src/lib/components/editor/field/field-root.svelte (2)
43-46
: Prop-to-state bridge: keep, but tiny clarity tweak optionalThe $effect correctly syncs the external prop into context state. If you want to avoid any TDZ confusion for readers, destructure props before registering the effect (no functional change).
-const fieldProps = usesFieldRoot(new FieldRootState(fieldLabelId)); -$effect(() => { - fieldProps.fieldId = fieldId; -}); +const fieldProps = usesFieldRoot(new FieldRootState(fieldLabelId)); +// after props destructure below, this will run with the correct value +$effect(() => { + fieldProps.fieldId = fieldId; +});
57-57
: Use Svelte style directive for grid-area
Replace the inline string style so the declaration is omitted whenfieldId
is undefined:-<div style="grid-area: {fieldId}" class={cn(...)} {...restProps}> +<div style:grid-area={fieldId} class={cn(...)} {...restProps}>All
FieldId
values (e.g.lexemeForm
,definition
,sentence
, etc.) are camelCase keys and conform to CSS custom-ident rules, so no additional sanitization is needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
frontend/viewer/src/lib/DialogsProvider.svelte
(0 hunks)frontend/viewer/src/lib/components/audio/AudioDialog.svelte
(4 hunks)frontend/viewer/src/lib/components/editor/field/field-root.svelte
(2 hunks)frontend/viewer/src/lib/components/editor/field/field-title.svelte
(1 hunks)frontend/viewer/src/lib/components/field-editors/audio-input.svelte
(4 hunks)frontend/viewer/src/lib/components/field-editors/multi-ws-input.svelte
(1 hunks)frontend/viewer/src/lib/components/field-editors/rich-multi-ws-input.svelte
(1 hunks)frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte
(8 hunks)frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte
(4 hunks)frontend/viewer/src/lib/entry-editor/object-editors/LexiconEditorPrimitive.svelte
(1 hunks)frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte
(5 hunks)frontend/viewer/src/lib/entry-editor/object-editors/subject-context.ts
(1 hunks)frontend/viewer/src/lib/layout/DevToolsDialog.svelte
(2 hunks)frontend/viewer/src/lib/services/dialogs-service.ts
(0 hunks)frontend/viewer/src/project/browse/BrowseView.svelte
(2 hunks)
💤 Files with no reviewable changes (2)
- frontend/viewer/src/lib/DialogsProvider.svelte
- frontend/viewer/src/lib/services/dialogs-service.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-30T04:53:41.702Z
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1844
File: frontend/viewer/src/lib/entry-editor/ItemListItem.svelte:26-37
Timestamp: 2025-07-30T04:53:41.702Z
Learning: In frontend/viewer/src/lib/entry-editor/ItemListItem.svelte, the TODO comments for unused props `index` and `actions` are intentional reminders for future work to be completed in a separate PR, not issues to be resolved immediately. These represent planned functionality that will be implemented later.
Applied to files:
frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte
📚 Learning: 2025-07-24T03:26:59.388Z
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1841
File: frontend/viewer/src/project/browse/filter/OpFilter.svelte:10-17
Timestamp: 2025-07-24T03:26:59.388Z
Learning: In Svelte 5, reactive statements use `$derived()` instead of the Svelte 4 `$:` syntax. For example, to make an array reactive to translation changes, use `const ops = $derived([...])` instead of `$: ops = [...]`.
Applied to files:
frontend/viewer/src/lib/components/editor/field/field-title.svelte
📚 Learning: 2025-08-14T12:50:25.135Z
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1906
File: frontend/viewer/src/lib/components/ui/dialog-shared/dialog-shared-root.svelte:3-3
Timestamp: 2025-08-14T12:50:25.135Z
Learning: In the dialog-shared-root.svelte component, the module-level `openDialogs` state is intentionally shared across all component instances to coordinate dialog stacking and overlay behavior across the entire application. This enables proper z-index management where newer dialogs appear on top and only the bottom dialog shows its overlay.
Applied to files:
frontend/viewer/src/lib/components/audio/AudioDialog.svelte
🧬 Code graph analysis (1)
frontend/viewer/src/lib/entry-editor/object-editors/subject-context.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IExampleSentence.ts (1)
IExampleSentence
(10-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: check-and-lint
- GitHub Check: Analyze (csharp)
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend-component-unit-tests
🔇 Additional comments (19)
frontend/viewer/src/lib/components/editor/field/field-title.svelte (1)
20-22
: Confirm single source of truth for label to avoid feedback loopsSetting
stateProps.label = label
in an effect is fine; just verifyuseFieldTitle()
doesn’t also derive/mutatelabel
internally, which could create a subtle loop. Nice use of Svelte 5 runes per our prior learning.frontend/viewer/src/lib/components/audio/AudioDialog.svelte (2)
87-90
: Good: validatemediaUri
before proceedingExplicitly throwing when
mediaUri
is missing prevents downstream null/undefined bugs.
136-139
: Public API: configurable title and header children look solidThe default title fallback and header snippet injection fit the PR’s “show context while recording” goal.
frontend/viewer/src/lib/components/field-editors/audio-input.svelte (4)
54-59
: Imports and new dependencies look correct.The new dialog and subject-context primitives are wired in cleanly.
66-73
: New wsLabel prop: API addition LGTM.Prop typing matches upstream usage; no issues spotted.
275-284
: Conditional rendering of AudioDialog is appropriate.Gated by supportsAudio and readonly; good UX boundary.
286-291
: Actions correctly open the recorder.Add/Replace routes now bind to the local dialog state as intended.
Also applies to: 344-345
frontend/viewer/src/lib/components/field-editors/rich-multi-ws-input.svelte (1)
80-81
: Pass-through of wsLabel is correct.Prop aligns with AudioInput API; no issues.
frontend/viewer/src/lib/components/field-editors/multi-ws-input.svelte (1)
55-56
: Pass-through of wsLabel is correct.Matches the new AudioInput prop; consistent with rich-multi variant.
frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte (1)
13-13
: Subject-context initialization looks correct.initSubjectContext(() => example) cleanly exposes the active example to consumers. No issues.
Also applies to: 30-30
frontend/viewer/src/lib/entry-editor/object-editors/EntryEditorPrimitive.svelte (1)
16-16
: Subject-context initialization for entry is solid.Also applies to: 36-36
frontend/viewer/src/lib/entry-editor/object-editors/LexiconEditorPrimitive.svelte (1)
1-13
: Prop typing and imports look correct for the union use-case.frontend/viewer/src/lib/entry-editor/object-editors/SenseEditorPrimitive.svelte (2)
15-15
: Subject-context initialization for sense is correct.Also applies to: 34-35
63-77
: Select wiring correctly syncs partOfSpeechId.Minor: If Select can emit rapid changes, consider batching assignment + onFieldChanged inside a microtask to avoid intermediate observers firing (only if you’ve seen jitter). Otherwise, LGTM.
frontend/viewer/src/lib/entry-editor/object-editors/subject-context.ts (1)
19-21
: Getter exposure via useSubjectContext is straightforward.If multiple primitives are nested (Entry → Sense → Example), confirm runed Context scoping behaves as expected (innermost wins) to ensure the audio dialog reads the most specific subject.
frontend/viewer/src/lib/components/editor/field/field-root.svelte (4)
3-15
: State class + nominal branding look solidGood use of a branded class with $state runes to enforce construction and provide reactive fields. No issues spotted.
23-23
: Type aliases alignedAliases updated to the new state type; consistent with the API shift.
Also applies to: 29-29
40-40
: Public props shape is clearAdding optional fieldId to props aligns with the new layout needs. Looks good.
17-20
: No breaking-change risk: all existing calls already pass FieldRootState instancesThe only usage of usesFieldRoot is within this file and invokes it with new FieldRootState(...), so no callers pass plain object literals.
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
@hahn-kev Can you add a screenshot for what it looks like when there is audio that is going to be replaced? Is it clear that the audio will be replaced (as opposed to having multiple recordings)? |
@imnasnainaec the UI doesn't look different in the recording page. But to get there they have to click "Replace Audio", so that should make it clear |
@hahn-kev Presumably it won't say "No audio" when there is audio. I was wondering how the audio is displayed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it 👍
I had put some effort into optimizing the context we show, so I put that in a draft PR.
Perhaps Sara will get to decide if that's worth merging too 😄. Obviously I'm also curious to hear any thoughts you have on it.
@imnasnainaec here's what replacing existing audio looks like I'm going to merge this in but I'm still interested in what you think. |
Ah, nice. And good use of space having the language abbrev. and corresponding gloss on the same line. In both the definition screenshot at top and this gloss screenshot, it feels a bit redundant to have the word "Definition" or "Gloss" at the start of both the title and the first line after the title. It appears that the second occurrence is only for the sake of the ? help icon. I guess that's |
Changed the audio recording dialog,

Before:
After:

above the recording area we display a readonly editor for the related object (example, sense, entry) and we only display the field that we're providing audio for (Definition in this case). In the title of the dialog is the field, and writing system that audio is being recorded for, not very useful here, but if there's multiple audio writing systems then it's very helpful.
Note that we're recording for Thai, but we're also displaying the Thai audio (which has no audio), this is to support the case where you're making a new recording of existing audio, you can easily compare your new recording with the old one, before you commit to replacing it.